Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore!: enable noUncheckedIndexedAccess #2006

Merged
merged 40 commits into from
Apr 18, 2023

Conversation

nickofthyme
Copy link
Collaborator

@nickofthyme nickofthyme commented Mar 23, 2023

Summary

Enables noUncheckedIndexedAccess across all code.

Details

Enabling the Typescript strict mode does not check for array index access (microsoft/TypeScript#39560) and assumes assessing a value at an array index produces the array/tuple type. However this can be deceiving and lead to errors in the code if this array access returns undefined.

Enabling this across the /src creates a consistent and more accurate type checking. This does require a bit more type guards to ensure the value is defined.

Note on tsconfig.json structures

So typescript/VScode changed how they respect nested tsconfig.json files. In the past, VScode would only look to the top-level tsconfig.json and ignore all other named-varients (i.e. tsconfig.lint.json) or nested tsconfig.json files (i.e. storybook/tsconfig.json).

Now this has changed, to where VScode will apply type checking using the nearest parent directory containing a tsconfig.json file. But the bad thing about this is that this assumes that tsconfig.json includes all child directories which may not be and often is not true! Thus making type checking a much bigger pain for it to be consistent between the IDE plugins and the tsc cli checks.

Ideally, we could apply certain compilerOptions to different directories and/or files much like eslint, because enabling noUncheckedIndexedAccess in test files is unnecessary and a little annoying.

To further complicate things, even if this did work, typescript automatically includes transitive imports thus requiring type checking on src code using different compilerOptions leading to conflicting linting errors like...

../packages/charts/src/chart_types/xy_chart/state/selectors/get_api_scale_configs.ts:93:9 - error TS2783: 'nice' is specified more than once, so this usage will be overwritten.

93         nice: false,
           ~~~~~~~~~~~

  ../packages/charts/src/chart_types/xy_chart/state/selectors/get_api_scale_configs.ts:96:9
    96         ...scaleConfigsByGroupId[groupId],
               ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    This spread always overwrites this property.

Here spreading the value of scaleConfigsByGroupId[groupId] In one case is assumed to always be defined thus thinks that nice is always overridden, and the other case it's possibly undefined.

So with that said, the best solution is just to apply the same compilerOptions throughout the code and avoid nested overly-constrained tsconfig.json files, in favor of tsconfig.<something>.json.

The next idea is to allow forced unwrapping array access (i.e. someThing[0]!.someProperty) everywhere except /src. But there are no linting rules to enforce this, the only semi-related rule is @typescript-eslint/no-unnecessary-type-assertion

Issues

close #1757

Checklist

  • The proper chart type label has been added (e.g. :xy, :partition)
  • All related issues have been linked (i.e. closes #123, fixes #123)
  • New public API exports have been added to packages/charts/src/index.ts
  • Unit tests have been added or updated to match the most common scenarios

@nickofthyme nickofthyme added :all Applies to all chart types internal Internal changes with no external impact labels Mar 23, 2023
@nickofthyme nickofthyme added the ci:skip Skip all build checks label Mar 23, 2023
@nickofthyme
Copy link
Collaborator Author

nickofthyme commented Mar 23, 2023

@markov00 Before I fix all the errors from this change, I wanted to get your take on this change? I have run into this more and more and I feel not enabling this makes the type checking very loose.

Example

A great example of this is fetching a single spec like this...

/**
* Returns first matching spec
* @internal
*/
export function getSpecFromStore<U extends Spec>(
specs: SpecList,
chartType: ChartType,
specType: string,
): U | undefined {
return (
(Object.values(specs).find((spec) => spec.chartType === chartType && spec.specType === specType) as U) ?? undefined
);
}

Currently if we call this function like...

const spec: HeatmapSpec = getSpecFromStore<HeatmapSpec>(specs, ChartType.Heatmap, SpecType.Series);

Typescript does NOT throw any errors and is completely fine with spec being HeatmapSpec type completely ignoring the possibility of undefined, even with the explicit return type!

The downsides I see to this are...

  • Accessing non-explicit tuple values from arrays. We use this paradigm in quite a few places, especially partition charts.
  • It assumes static array values can be undefined, which is understandable cuz they could change. This is not too common with our code.

@markov00
Copy link
Member

@nickofthyme I totally agree with these changes, I see the benefit and I think is desirable, go on with it please

- getSpecFromStore option to require spec or throw
- getSpecFromStore option to optionally get spec or null
@nickofthyme nickofthyme removed the ci:skip Skip all build checks label Mar 29, 2023
Newer versions of typescript/vscode now support multiple tsconfig.json
files in a single project. Previously the base tsconfig.json was the only
file used and all others were ignored. Now if a tsconfig.json file is at
the root of a directory that will be applied to all child directories.
This change was causing failures in cli but not is vscode for test files.
This index access makes it easier to type and describe the different variants
This is required as a limitation of typescript/vscode not permitting nested tsconfigs
here we favor the use of forced unwrapping (i.e. `d[0]!`) for less strict checking
here we favor the use of forced unwrapping (i.e. `d[0]!`) for less strict checking
Copy link
Member

@markov00 markov00 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just code review (I skipped the changes in Storybook because it looks relatively safe).

Co-authored-by: Marco Vettorello <vettorello.marco@gmail.com>
Copy link
Member

@markov00 markov00 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good to merge when CI is green.
One thing: for safety reasons what if we merge this as a breaking change and generate a release just before merging this? it could be easy to track/rollback in case of failures

@nickofthyme nickofthyme merged commit 2734de5 into elastic:main Apr 18, 2023
@nickofthyme nickofthyme deleted the enable-no-unchecked-access branch April 18, 2023 16:38
@nickofthyme nickofthyme changed the title chore: enable noUncheckedIndexedAccess chore!: enable noUncheckedIndexedAccess Apr 18, 2023
nickofthyme added a commit that referenced this pull request Apr 18, 2023
enables stricter types for array access.

BREAKING CHANGE: Enables stricter type option in src and could have
unexpected changes. This release is meant to serve as a clean break
in case any issues arise.

---------

Co-authored-by: Marco Vettorello <vettorello.marco@gmail.com>
nickofthyme pushed a commit that referenced this pull request Apr 18, 2023
## [56.0.1](v56.0.0...v56.0.1) (2023-04-18)

### Code Refactoring

* enable `noUncheckedIndexedAccess` ([#2006](#2006)) ([2f70349](2f70349))

### BREAKING CHANGES

* Enables stricter type option in src and could have
unexpected changes. This release is meant to serve as a clean break
in case any issues arise.
nickofthyme added a commit that referenced this pull request Apr 19, 2023
enables stricter types for array access.

BREAKING CHANGE: Enables stricter type option in src and could have
unexpected changes. This release is meant to serve as a clean break
in case any issues arise.
nickofthyme pushed a commit that referenced this pull request Apr 19, 2023
## [56.0.1](v56.0.0...v56.0.1) (2023-04-19)

### Code Refactoring

* enable `noUncheckedIndexedAccess` ([#2006](#2006)) ([f446cca](f446cca))

### BREAKING CHANGES

* Enables stricter type option in src and could have
unexpected changes. This release is meant to serve as a clean break
in case any issues arise.
nickofthyme pushed a commit that referenced this pull request Apr 19, 2023
# [57.0.0](v56.0.0...v57.0.0) (2023-04-19)

### Code Refactoring

* enable `noUncheckedIndexedAccess` ([#2006](#2006)) ([f446cca](f446cca))

### BREAKING CHANGES

* Enables stricter type option in src and could have
unexpected changes. This release is meant to serve as a clean break
in case any issues arise.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:all Applies to all chart types internal Internal changes with no external impact
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add strict index checking to array access
2 participants